-
Notifications
You must be signed in to change notification settings - Fork 2.2k
FINERACT-2412: Full term tranche - Schedule handling and Calculations #5294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FINERACT-2412: Full term tranche - Schedule handling and Calculations #5294
Conversation
ff77199 to
ee3752c
Compare
ee3752c to
926421f
Compare
| final LocalDate firstDisbursedPeriodStartDate = firstDisbursedPeriod.isPresent() ? firstDisbursedPeriod.get().getFromDate() | ||
| : scheduleModel.getMaturityDate(); | ||
|
|
||
| final LoanApplicationTerms loanApplicationTerms = new LoanApplicationTerms.Builder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this LoanAplicationBuilder Logic must be extracted to an new helper function. So that it is easier to read.
and group this chaining maybe like this?
// Loan basics
.currency(...)
.principal(...)
.repaymentsStartingFromDate(...)
// Term & frequency
.loanTermFrequency(...)
.numberOfRepayments(...)
.repaymentEvery(...)
.repaymentPeriodFrequencyType(...)
// Interest configuration
.interestRatePerPeriod(...)
.annualNominalInterestRate(...)
.interestRatePeriodFrequencyType(...)
.interestMethod(...)
// Day count conventions
.daysInMonthType(...)
.daysInYearType(...)
.daysInYearCustomStrategy(...)
// Flags & options
.isDownPaymentEnabled(false)
.allowPartialPeriodInterestCalculation(...)
// Technical
.mc(mc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
926421f to
cdaf5bc
Compare
Aman-Mittal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code wise seems ok, Not reviewed based on e2e tests due to my lack of functional knowlegede.
Message to Maintainer: Please cross-check the tests before merge
8ddfbe7 to
1c4cb36
Compare
| Money amortizableAmount = disbursementTransaction.getAmount(currency).minus(downPaymentAmount); | ||
| emiCalculator.addDisbursement(model, transactionDate, amortizableAmount); | ||
|
|
||
| if (loan.isAllowFullTermForTranche() && loan.getNumberOfRepayments() > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are already updating the installments by model as part of recalculateRepaymentPeriodsWithEMICalculation. Do we really need to do it again here?
One of them should be removed, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| allocateOverpayment(disbursementTransaction, transactionCtx); | ||
| } | ||
|
|
||
| private void updateInstallmentsByModel(final LoanTransaction loanTransaction, final ProgressiveTransactionCtx ctx) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check whether recalculateRepaymentPeriodsWithEMICalculation and updateInstallmentsByModel is duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed updateInstallmentsByModel
| if (outstandingBalanceCalculation == null) { | ||
| outstandingBalanceCalculation = Memo.of(() -> { | ||
| if (getInterestPeriods().isEmpty()) { | ||
| return getZero(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would be the outstanding loan balance zero if there are no interest periods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
...n/src/main/java/org/apache/fineract/portfolio/loanproduct/calc/ProgressiveEMICalculator.java
Show resolved
Hide resolved
| .mc(mc).build(); | ||
| } | ||
|
|
||
| private void mergeNewInterestScheduleModelWithExistingOne(final ProgressiveLoanInterestScheduleModel scheduleModel, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misleading name, use something like this instead: mergeNewScheduleModelWithExistingOne
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a mergeNewInterestScheduleModelWithExistingOne function. Is his duplicate? Can we have just one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mergeNewInterestScheduleModelWithExistingOne for re-aging actually has the different approach, I renamed it accordinally
| * * Generates temporary interestScheduleModel with particular disbursement date | ||
| */ | ||
| @NotNull | ||
| private ProgressiveLoanInterestScheduleModel generateTemporaryScheduleModelWithParticularDisbursementDate( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a generateTemporaryReAgedScheduleModel function. Is this duplicate? Can we have just one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right, left just one of them
adamsaghy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kindly see my review!
1c4cb36 to
b817f5b
Compare
| loanTransaction.getLoan(), installmentCounter.get(), rm.getFromDate(), rm.getDueDate(), | ||
| rm.getDuePrincipal().getAmount(), null, null, null, null, null, null, null, false, false, false); | ||
|
|
||
| if (rm.getDueInterest().isGreaterThanZero()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We dont need this. Set interest in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| installment.updateObligationsMet(currency, transactionDate); | ||
| } else { | ||
| if (model.loanProductRelatedDetail().isAllowFullTermForTranche() | ||
| && loanTransaction.getLoan().getNumberOfRepayments() > 0 && !rm.getDueDate().isBefore(transactionDate)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can fetch this from the model.loanProductRelatedDetail()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
b817f5b to
d931e77
Compare
d931e77 to
58f9ddc
Compare
Description
Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
Your assigned reviewer(s) will follow our guidelines for code reviews.